Config map nim model name fetch #3276

olavtar · 2024-10-01T01:08:24Z

Description

Retrieving ConfigMap using service account and added ${uid} to PVC name to avoid waiting for resource termination after deletion

How Has This Been Tested?

Tested locally.

Test Impact

Deployed a NIM model, deleted it, and deployed another model while the PVC was still in the 'Terminating' state.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

Signed-off-by: Olga Lavtar <[email protected]>

… name to avoid waiting for resource termination after deletion Signed-off-by: Olga Lavtar <[email protected]>

openshift-ci · 2024-10-01T01:08:37Z

Hi @olavtar. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

codecov · 2024-10-01T01:51:44Z

Codecov Report

Attention: Patch coverage is 59.25926% with 11 lines in your changes missing coverage. Please review.

Project coverage is 84.81%. Comparing base (228e221) to head (bc3c9ac).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
frontend/src/api/k8s/servingRuntimes.ts	66.66%	5 Missing ⚠️
...rc/pages/modelServing/screens/projects/nimUtils.ts	42.85%	4 Missing ⚠️
...projects/NIMServiceModal/DeployNIMServiceModal.tsx	0.00%	1 Missing ⚠️
...s/projects/NIMServiceModal/NIMModelListSection.tsx	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3276      +/-   ##
==========================================
- Coverage   84.87%   84.81%   -0.06%     
==========================================
  Files        1306     1306              
  Lines       29196    29207      +11     
  Branches     7885     7890       +5     
==========================================
- Hits        24780    24773       -7     
- Misses       4416     4434      +18

Files with missing lines	Coverage Δ
...d/src/pages/modelServing/screens/projects/utils.ts	`97.27% <100.00%> (+0.42%)`	⬆️
...projects/NIMServiceModal/DeployNIMServiceModal.tsx	`4.95% <0.00%> (-1.00%)`	⬇️
...s/projects/NIMServiceModal/NIMModelListSection.tsx	`1.63% <0.00%> (ø)`
...rc/pages/modelServing/screens/projects/nimUtils.ts	`52.27% <42.85%> (ø)`
frontend/src/api/k8s/servingRuntimes.ts	`94.39% <66.66%> (-4.55%)`	⬇️

... and 6 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 228e221...bc3c9ac. Read the comment docs.

andrewballantyne · 2024-10-01T13:04:15Z

/ok-to-test

andrewballantyne

None of these are critical blockers. Your dodge a typescript issue because Secrets & Configmaps are very close in structure.

I'm going to approve this to start updating downstream. Please fire up another PR and fix this in main asap.

andrewballantyne · 2024-10-02T14:24:05Z

frontend/src/pages/modelServing/screens/projects/NIMServiceModal/NIMModelListSection.tsx

@@ -32,7 +32,7 @@ const NIMModelListSection: React.FC<NIMModelListSectionProps> = ({
  useEffect(() => {
    const getModelNames = async () => {
      try {
-        const modelInfos = await fetchNIMModelNames(dashboardNamespace);
+        const modelInfos = await fetchNIMModelNames();


Your hook still has dashboardNamespace as a dependency... I think you're no longer using it in the hook... so please remove it.

andrewballantyne · 2024-10-02T14:26:55Z

frontend/src/pages/modelServing/screens/projects/nimUtils.ts

@@ -9,9 +9,9 @@ const NIM_NGC_SECRET_NAME = 'nvidia-nim-image-pull';
 export const getNGCSecretType = (isNGC: boolean): string =>
  isNGC ? 'kubernetes.io/dockerconfigjson' : 'Opaque';

-const getNIMSecretData = async (secretName: string): Promise<SecretKind> => {
+export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {


Not just a SecretKind anymore

Suggested change

export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {

export const getNIMResource = async <T extends K8sResourceCommon = SecretKind>(resourceName: string): Promise<T> => {

In your one location you call this, you'll want to call it like:

const configMap = await getNIMResource<ConfigMapKind>(NIM_CONFIGMAP_NAME);

See my next comment.

andrewballantyne · 2024-10-02T14:38:45Z

frontend/src/pages/modelServing/screens/projects/utils.ts

-): Promise<ModelInfo[] | undefined> => {
-  const configMap = await getConfigMap(dashboardNamespace, NIM_CONFIGMAP_NAME);
+export const fetchNIMModelNames = async (): Promise<ModelInfo[] | undefined> => {
+  const configMap = await getNIMResource(NIM_CONFIGMAP_NAME);


Suggested change

const configMap = await getNIMResource(NIM_CONFIGMAP_NAME);

const configMap = await getNIMResource<ConfigMapKind>(NIM_CONFIGMAP_NAME);

andrewballantyne · 2024-10-02T15:04:30Z

frontend/src/api/k8s/servingRuntimes.ts

@@ -171,8 +181,33 @@ export const assembleServingRuntime = (
    volumes.push(getshmVolume('2Gi'));
  }

-  updatedServingRuntime.spec.volumes = volumes;
+  if (nimPVCName) {


This code should never have been here -- you folks need to work on lowering your points of contact when fixing a blocker fix. You snuck this PVC thing into a high trafficked area for 2 other flows you're not touching instead of just adding this to your serving runtime template before ever calling this flow.

This change has already been tested and I'm going to let it in. But please do not sneak more "nice to have" code in when you need to fix critical blocking issues.

andrewballantyne · 2024-10-02T15:07:46Z

frontend/src/api/k8s/servingRuntimes.ts

@@ -242,6 +277,7 @@ export const updateServingRuntime = (options: {
  initialAcceleratorProfile?: AcceleratorProfileState;
  selectedAcceleratorProfile?: AcceleratorProfileSelectFieldState;
  isModelMesh?: boolean;
+  nimPVCName?: string;


This was unnecessary -- you don't support edit, updating the serving runtime is not done currently.

openshift-ci · 2024-10-02T15:10:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [andrewballantyne]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* fix: retrieve ConfigMap using service account Signed-off-by: Olga Lavtar <[email protected]> * fix: retrieve ConfigMap using service account and added ${uid} to PVC name to avoid waiting for resource termination after deletion Signed-off-by: Olga Lavtar <[email protected]> --------- Signed-off-by: Olga Lavtar <[email protected]>

* fix: retrieve ConfigMap using service account * fix: retrieve ConfigMap using service account and added ${uid} to PVC name to avoid waiting for resource termination after deletion --------- Signed-off-by: Olga Lavtar <[email protected]> Co-authored-by: olavtar <[email protected]>

Config map nim model name fetch (opendatahub-io#3276) (opendatahub-io#3290)

olavtar added 2 commits September 30, 2024 14:44

fix: retrieve ConfigMap using service account

fac3687

Signed-off-by: Olga Lavtar <[email protected]>

fix: retrieve ConfigMap using service account and added ${uid} to PVC…

bc3c9ac

… name to avoid waiting for resource termination after deletion Signed-off-by: Olga Lavtar <[email protected]>

openshift-ci bot requested review from alexcreasy and mturley October 1, 2024 01:08

openshift-ci bot added the needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI label Oct 1, 2024

olavtar marked this pull request as draft October 1, 2024 01:11

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Oct 1, 2024

openshift-ci bot added ok-to-test The openshift bot needs `ok-to-test` to allow non member PRs to run the tests. and removed needs-ok-to-test The openshift bot needs to label PRs from non members to avoid strain on the CI labels Oct 1, 2024

olavtar marked this pull request as ready for review October 1, 2024 18:43

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Oct 1, 2024

openshift-ci bot requested review from dpanshug and ppadti October 1, 2024 18:43

andrewballantyne approved these changes Oct 2, 2024

View reviewed changes

openshift-ci bot assigned andrewballantyne Oct 2, 2024

openshift-ci bot added the lgtm label Oct 2, 2024

openshift-ci bot added the approved label Oct 2, 2024

openshift-merge-bot bot merged commit 33ea696 into opendatahub-io:main Oct 2, 2024
10 checks passed

andrewballantyne added a commit to red-hat-data-services/odh-dashboard that referenced this pull request Oct 2, 2024

Merge pull request #355 from opendatahub-io/v2.27.0-fixes

cb94ecb

Config map nim model name fetch (opendatahub-io#3276) (opendatahub-io#3290)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Config map nim model name fetch #3276

Config map nim model name fetch #3276

olavtar commented Oct 1, 2024

openshift-ci bot commented Oct 1, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading

andrewballantyne commented Oct 1, 2024

andrewballantyne left a comment

andrewballantyne Oct 2, 2024

andrewballantyne Oct 2, 2024

andrewballantyne Oct 2, 2024

andrewballantyne Oct 2, 2024

andrewballantyne Oct 2, 2024

openshift-ci bot commented Oct 2, 2024

	export const getNIMResource = async (resourceName: string): Promise<SecretKind> => {
	export const getNIMResource = async <T extends K8sResourceCommon = SecretKind>(resourceName: string): Promise<T> => {

	const configMap = await getNIMResource(NIM_CONFIGMAP_NAME);
	const configMap = await getNIMResource<ConfigMapKind>(NIM_CONFIGMAP_NAME);

Config map nim model name fetch #3276

Config map nim model name fetch #3276

Conversation

olavtar commented Oct 1, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

openshift-ci bot commented Oct 1, 2024

codecov bot commented Oct 1, 2024 • edited Loading

Codecov Report

andrewballantyne commented Oct 1, 2024

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne Oct 2, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 2, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 2, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 2, 2024

Choose a reason for hiding this comment

andrewballantyne Oct 2, 2024

Choose a reason for hiding this comment

openshift-ci bot commented Oct 2, 2024

codecov bot commented Oct 1, 2024 •

edited

Loading